Skip to content

perf(array): add dense fast path to Array.prototype.push#5300

Open
tkshsbcue wants to merge 1 commit intoboa-dev:mainfrom
tkshsbcue:perf/hoist-array-object-clone-out-of-loops
Open

perf(array): add dense fast path to Array.prototype.push#5300
tkshsbcue wants to merge 1 commit intoboa-dev:mainfrom
tkshsbcue:perf/hoist-array-object-clone-out-of-loops

Conversation

@tkshsbcue
Copy link
Copy Markdown
Contributor

@tkshsbcue tkshsbcue commented Apr 5, 2026

perf(array): add dense fast path to Array.prototype.push

Description

Array.prototype.push goes through the generic [[Set]] path for every element — converting the index to a PropertyKey, cloning the receiver, calling __get_own_property__, then __define_own_property__ with full descriptor validation. For dense arrays, none of this is necessary.

The PushValueToArray opcode (used for array literals like [1, 2, 3]) already has a fast path that appends directly to dense indexed storage via push_dense(). This PR applies the same pattern to the Array.prototype.push builtin.

How it works

Before entering the generic [[Set]] loop, check if the object is an extensible array with dense storage. If so, push each element directly via push_dense() and update the length in-place. push_dense() handles type transitions (DenseI32 → DenseF64 → DenseElement) internally and only returns false for already-sparse storage, so the first push serves as a probe — if it succeeds, all subsequent pushes are guaranteed to succeed.

The fast path is skipped for:

  • Non-array objects (array-like objects go through the slow path)
  • Non-extensible arrays (Object.preventExtensions, Object.freeze, Object.seal)
  • Sparse arrays (storage already transitioned to sparse)
  • Arrays whose length doesn't fit in i32

The slow path is completely untouched.

Benchmark results

Criterion microbenchmark (200 iterations × 5000 integer pushes):

Baseline (3 runs):          127.48 ms median
With fast path (3 runs):     64.73 ms median

This is a push-dominated benchmark. On a more realistic workload (pushing objects + filtering), the improvement is smaller since push is no longer the bottleneck:

Realistic (objects + filter, Date.now(), release build):
  Baseline:    139 ms median
  With patch:  110 ms median

Improvement ranges from ~20% (mixed workloads) to ~49% (push-dominated) depending on how much time is spent in push.

@tkshsbcue tkshsbcue requested a review from a team as a code owner April 5, 2026 08:17
@github-actions github-actions bot added C-Builtins PRs and Issues related to builtins/intrinsics Waiting On Review Waiting on reviews from the maintainers labels Apr 5, 2026
@github-actions github-actions bot added this to the v1.0.0 milestone Apr 5, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 5, 2026

Test262 conformance changes

Test result main count PR count difference
Total 53,125 53,125 0
Passed 50,827 50,824 -3
Ignored 1,482 1,482 0
Failed 816 819 +3
Panics 0 0 0
Conformance 95.67% 95.67% -0.01%
Broken tests (3):
test/staging/sm/Array/set-with-indexed-property-on-prototype-chain.js (previously Passed)
test/built-ins/Array/prototype/push/set-length-array-length-is-non-writable.js (previously Passed)
test/built-ins/Array/prototype/push/set-length-array-is-frozen.js (previously Passed)

Tested main commit: 442b7fd5840898565a48be4adb70b8dbbfbff84c
Tested PR commit: 372c580b4dd1a7b769b45283fe1b123623328825
Compare commits: 442b7fd...372c580

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 5, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.64%. Comparing base (6ddc2b4) to head (372c580).
⚠️ Report is 932 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #5300       +/-   ##
===========================================
+ Coverage   47.24%   59.64%   +12.40%     
===========================================
  Files         476      589      +113     
  Lines       46892    63588    +16696     
===========================================
+ Hits        22154    37930    +15776     
- Misses      24738    25658      +920     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tkshsbcue tkshsbcue force-pushed the perf/hoist-array-object-clone-out-of-loops branch 2 times, most recently from 447ebbe to 39e6041 Compare April 6, 2026 05:02
@github-actions github-actions bot added C-Documentation update documentation C-Benchmark Issues and PRs related to the benchmark subsystem. C-Javascript Pull requests that update Javascript code labels Apr 6, 2026
Array.prototype.push was going through the generic [[Set]] machinery
for every element, which involves property descriptor validation and
prototype chain walks. This is unnecessary for dense arrays where we
can append directly to the indexed storage.

Add a fast path that mirrors the PushValueToArray opcode: verify the
array is extensible and its shape matches the default array template
(guaranteeing length is writable), then push directly via push_dense()
and update the length in-place. Falls through to the existing slow
path for sparse arrays, non-extensible arrays, arrays with modified
property descriptors, and array-like objects.

~49% improvement on a push-dominated microbenchmark, ~20% on a
realistic mixed workload (push objects + filter).
@tkshsbcue tkshsbcue force-pushed the perf/hoist-array-object-clone-out-of-loops branch from 39e6041 to 372c580 Compare April 6, 2026 05:03
@zhuzhu81998
Copy link
Copy Markdown
Contributor

the failing tests hint on skipped checks (similar to #5076)

@tkshsbcue
Copy link
Copy Markdown
Contributor Author

tkshsbcue commented Apr 13, 2026

You're right @zhuzhu81998 same class of bug as #5076. The shape check only verifies the array's own shape matches the default template, but doesn't guard against indexed property setters on the prototype chain. So when someone does Object.defineProperty(Array.prototype, "0", { set: ... }), the fast path fires anyway and bypasses [[Set]] entirely, skipping the setter invocation and any side effects (like freezing the array or making length non-writable).

I think the most practical fix is to extend the guard to also check that Array.prototype and Object.prototype still have their default template shapes if anyone has defined an indexed property on either, the shape will differ and we fall through to the slow path. Something like:

let array_proto_shape = context
    .intrinsics()
    .constructors()
    .array()
    .prototype();

// bail if Array.prototype or Object.prototype have been modified
// with indexed properties

The more robust long-term approach would be a realm-level "no indexed accessors on prototypes" flag (like V8's protector cells), but checking prototype shapes should be sufficient here and keeps the PR scoped.

I'll update the PR with the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-Benchmark Issues and PRs related to the benchmark subsystem. C-Builtins PRs and Issues related to builtins/intrinsics C-Documentation update documentation C-Javascript Pull requests that update Javascript code Waiting On Review Waiting on reviews from the maintainers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants